-
Notifications
You must be signed in to change notification settings - Fork 192
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: add status code handling for delete endpoints #1074
fix: add status code handling for delete endpoints #1074
Conversation
go/rtl/auth.go
Outdated
if res.StatusCode == 204 { // for delete endpoints there's no response body | ||
return nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is the essential part. The others comes from go fmt
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you change the comment to "204 No Content. DELETE endpoints returns response with no body"?
Hi, could you review this changes? |
@jeremytchang Hi, could you review this change? |
@jkaster Hi, cloud you review this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LG%C. Nitpick on the comment.
Good catch, though, which endpoint did this fail on?
We have integration tests which delete dashboard, user, and look, and they all pass.
We may need an additional integration test on the endpoint that caused this.
go/rtl/auth.go
Outdated
if res.StatusCode == 204 { // for delete endpoints there's no response body | ||
return nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you change the comment to "204 No Content. DELETE endpoints returns response with no body"?
@jeremytchang Thank you for the review! I'll create a example to reproduce the error and add integration test if it is needed (maybe this weekend). |
@jeremytchang Here's the minimum example of reproducing error (need to run this on the local PC). https://go.dev/play/p/EFRxP79d9z9 This example fails with I'll implement integration test for this next. |
integration test added in 3a4cdcc |
@jeremytchang Could you approve to run github actions workflow for confirming the IT run correctly? |
@jeremytchang Could you approve to run CI? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM if integration tests pass
Thanks Jeremy! CI is failed by permission denied on the step of docker pull, but I didn't any change about this part. |
22.8 shouldn't be part of the integration test run now. I've requested a re-run of all CI tests |
Hmm. Looks like we have an issue in CI right now |
Rebasing branch to see if it fixes the CI issue. |
Hmmm. Required Checks is failed, too. |
Hey @hirosassa! Our CI can only run on branches in our repo because of security reasons. I've copied your changes to this PR #1193. When we squash merge the PR, i'll add you as co-author of the squashed commit. |
e5d3545
to
9534bd8
Compare
Changes copied from approved PR from forked repo #1074 Original commit message: I added status code handling in the Go SDK to deal with delete endpoints' response body. In Looker API, delete endpoints call returns empty response body with status code 204 when the API call is succeeded. Such API call will be failed with EOF error in the current implementation because of reading empty response body. Co-authored-by: hirohito-sasakawa <hirohito-sasakawa@m3.com> Co-authored-by: hirosassa <hiro.sassa@gmail.com>
I'll close this PR coz the problem is fixed in #1193. |
I added status code handling in the Go SDK to deal with delete endpoints' response body.
In Looker API, delete endpoints call returns empty response body with status code 204 when the API call is succeeded.
Such API call will be failed with
EOF
error in the current implementation because of reading empty response body.Developer Checklist ℹ️
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly: